Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New syntax #225

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

New syntax #225

wants to merge 10 commits into from

Conversation

rustamwin
Copy link
Member

@rustamwin rustamwin commented Nov 9, 2023

Q A
Is bugfix?
New feature? ✔️
Breaks BC? ✔️
Fixed issues #193, #171
- use Yiisoft\Router\Route;
- use Yiisoft\Router\Group;
+ use Yiisoft\Router\Builder\RouteBuilder as Route;
+ use Yiisoft\Router\Builder\GroupBuilder as Group;


Group::create()->routes(
    Route::get('')->name('index'),
-   Route::get('/posts')->name('posts'),
+   Route::get('/posts')->name('posts'), // No getters, only setters
+   new Route(['GET'], '/'),
);

@rustamwin rustamwin added the status:code review The pull request needs review. label Nov 9, 2023
@rustamwin rustamwin requested a review from a team November 9, 2023 07:22
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0892cbe) 95.54% compared to head (782bf91) 96.35%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #225      +/-   ##
============================================
+ Coverage     95.54%   96.35%   +0.80%     
- Complexity      174      234      +60     
============================================
  Files            14       16       +2     
  Lines           561      685     +124     
============================================
+ Hits            536      660     +124     
  Misses           25       25              
Files Coverage Δ
src/Builder/GroupBuilder.php 100.00% <100.00%> (ø)
src/Builder/RouteBuilder.php 100.00% <100.00%> (ø)
src/CurrentRoute.php 100.00% <100.00%> (ø)
src/Debug/RouterCollector.php 100.00% <100.00%> (ø)
src/Group.php 100.00% <100.00%> (ø)
src/Middleware/Router.php 100.00% <100.00%> (ø)
src/Route.php 100.00% <100.00%> (ø)
src/RouteCollection.php 100.00% <100.00%> (ø)
src/RouteCollector.php 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

what-the-diff bot commented Nov 9, 2023

PR Summary

This pull request introduces numerous updates aimed at better organizing and streamlining how routes are handled in the system:

  • New Classes

    • Two new classes, GroupBuilder and RouteBuilder, have been added. These classes represent a group of routes and a single route respectively, and they include methods for defining diverse route characteristics, such as HTTP methods, route pattern, middlewares, hosts, and more.
  • Method and Property Modifications

    • Several existing methods (getName(), getHost(), getPattern(), getMethods()) in the CurrentRoute.php file and Debug/RouterCollector.php were adjusted to use new direct methods instead of a generic getData() method.
    • In the Group.php file, some properties were removed and replaced with new ones, and a variety of new methods designed for handling these properties were added.
  • “Route”

    • The Route.php file underwent a major update where new properties were added, methods were adjusted to utilize these new properties, and unnecessary methods were removed. The changes aim to streamline the usage of the class and make it more efficient.
  • Route Collection Update

    • The RouteCollection.php was refactored to better use new methods in the Route class and to remove outdated code.
  • Route Collector Update

    • The RouteCollector.php underwent several property and method name changes and had type assertions and argument validations added to improve code robustness and reliability.
  • Inclusion of Test Cases

    • A new test file for RouteBuilder class was added and several existing test files were updated to comply with the changes made to the core classes.
  • Miscellaneous

    • Adjustments were made in RouteCollectionTest.php and RouteCollectorTest.php to reflect changes made in the main classes.

These changes all contribute to a more robust and efficient route handling system within the software, aligning the codebase with modern best practices in PHP coding.

Copy link
Member

@vjik vjik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Update code of conduct #191 suggest use named arguments, and this must improve performance. In this PR keep routes/groups object and additional add new routes/groups objects with named arguments. Seems, this decrease performance...

  2. This not fix Add friendly exception with examples for exception "middleware() can not be used after action()" #171.

@rustamwin
Copy link
Member Author

In this PR keep routes/groups object and additional add new routes/groups objects with named arguments. Seems, this decrease performance...

We can mention it in docs or drop the current syntax.

This not fix #171.

Yes, doesn't fix directly. Since there is no exception, there is no need for it.

@vjik
Copy link
Member

vjik commented Nov 22, 2023

We can mention it in docs or drop the current syntax.

But what goal of syntax change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review The pull request needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants